Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Genre Graph for YIM 2024 #3087

Merged
merged 9 commits into from
Dec 20, 2024
Merged

Add Genre Graph for YIM 2024 #3087

merged 9 commits into from
Dec 20, 2024

Conversation

anshg1214
Copy link
Member

No description provided.

@pep8speaks
Copy link

pep8speaks commented Dec 18, 2024

Hello @anshg1214! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-12-20 17:57:16 UTC

@anshg1214 anshg1214 requested a review from MonkeyDo December 19, 2024 16:26
@MonkeyDo MonkeyDo force-pushed the ansh/add-yim-genre-graph branch from f290fa5 to 9d5a399 Compare December 19, 2024 20:44
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking nice!

We might want to adjust the colors to use the ones defined in the YIM24 file (the "text" colors), with variations, maybe?
https://github.com/metabrainz/listenbrainz-server/blob/yim-2024/frontend/js/src/user/year-in-music/2024/YearInMusic2024.tsx#L63-L68

Try to tie it up with the rest of the page look.
That being said I'm going to merge this to avoid further merge conflicts.

The tag counts don't look right to me at all, as they all have crazy number 5 times higher than my actual listen count for the year. Not sure there's anything we can do here though.

@MonkeyDo
Copy link
Member

MonkeyDo commented Dec 20, 2024

Oops, not merging, that test is getting stuck. I saw you were talking with lucifer yesterday, now I see the issue.
I'll let you figure that one out :0

@amCap1712 amCap1712 force-pushed the ansh/add-yim-genre-graph branch from d4f4858 to b4bc18c Compare December 20, 2024 15:11
@anshg1214
Copy link
Member Author

We might want to adjust the colors to use the ones defined in the YIM24 file (the "text" colors), with variations, maybe?

I had that in mind, but if we do it the graph does not look good.
image

The tag counts don't look right to me at all, as they all have crazy number 5 times higher than my actual listen count for the year. Not sure there's anything we can do here though.

@amCap1712 As I can see in the SQL query, instead of merging the genre count from recording, artist and release group, can we have that data separate?

@MonkeyDo
Copy link
Member

We might want to adjust the colors to use the ones defined in the YIM24 file (the "text" colors), with variations, maybe?

I had that in mind, but if we do it the graph does not look good. image

I meant more using the four colors, regardless of the chosen season. So where other graph use the selected season's textColor, this graph would use all 4 textColor from the seasons. I reckon we might still have the same issue of not enough variety between colours, but maybe it works?

@anshg1214
Copy link
Member Author

This looks good!

image

@anshg1214 anshg1214 merged commit 5e45f6e into yim-2024 Dec 20, 2024
2 checks passed
@anshg1214 anshg1214 deleted the ansh/add-yim-genre-graph branch December 20, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants